Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: skip readonly properties on entities when generating factories #798

Merged
merged 5 commits into from
Feb 1, 2025

Conversation

KDederichs
Copy link
Contributor

@KDederichs KDederichs commented Jan 26, 2025

Fixes #769 by checking if the property is writable or settable via constructor

Things that are missing:

  • Test cases (as soon as I figure out how those maker tests work)
  • always_force_property (I guess that needs to be injected there since it's only present in Hydrator and Instantiator)

If you got some pointers for those two points, I'd gladly take those

@KDederichs
Copy link
Contributor Author

@nikophil What would be the preferred way to get the always_force_properties into the MakeFactoryData?

My thought would be inject it into FactoryGenerator (.zenstruck_foundry.maker.factory.generator) with the ContainerBuilder and then use that to get that into the MakeFactoryData via setter.

Any thoughts on this?

@nikophil
Copy link
Member

nikophil commented Jan 26, 2025

Hi! Sorry I currently don't have access to a computer...
Yeah I'd do it like this I think, and pass it to its constructor or to getDefaultProperties()

@KDederichs
Copy link
Contributor Author

Hmm any insights on how to actually add a testcase for that? I don't think it ever loads any of the config options in the tests.

Another good option for this I just thought of was to not connect it to that flag in the config but to a new command line argument.
I think that might be even better tbh, cause that way the user can decide on a class by class basis if they want to skip those or not.

Copy link
Member

@nikophil nikophil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @KDederichs

thanks for this PR 👍

Another good option for this I just thought of was to not connect it to that flag in the config but to a new command line argument.
I think that might be even better tbh, cause that way the user can decide on a class by class basis if they want to skip those or not.

I have some doubt that people will ever use such niche option, but I'm not opposed to add it :)
What do you think of doing both? If always_force_properties is enabled, we will never skip any properties, and if the option is added, we will not skip those properties for this factory. Using the option AND having the configuration enabled would be redundant but won't trigger any warning nor error.

Hmm any insights on how to actually add a testcase for that? I don't think it ever loads any of the config options in the tests.

We don't have any mechanism for this yet. You can check what's done in zenstruck/messenger-test (see here and here) about this. Or if you're not willing to do this, I can merge your PR without this test, and I'll add it myself

src/Maker/Factory/MakeFactoryData.php Outdated Show resolved Hide resolved
src/Maker/Factory/MakeFactoryData.php Outdated Show resolved Hide resolved
@KDederichs
Copy link
Contributor Author

Well not quite sure what PHPStan wants from me but I think that's how the always_force_property stuff should work.
I also figured out how to test it, turn's out .zenstruck_foundry.maker.factory.generator doesn't seem to be a thing.

If you know how to best inject that setting into there let me know

src/Maker/Factory/FactoryGenerator.php Outdated Show resolved Hide resolved
src/Maker/Factory/FactoryGenerator.php Outdated Show resolved Hide resolved
src/Maker/Factory/MakeFactoryData.php Outdated Show resolved Hide resolved
src/ZenstruckFoundryBundle.php Outdated Show resolved Hide resolved
@KDederichs KDederichs marked this pull request as ready for review January 29, 2025 21:04
Copy link
Member

@nikophil nikophil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! this was harder than expected!

@KDederichs
Copy link
Contributor Author

Yeah many little hidden pitfalls, I did not expect this either

@nikophil nikophil merged commit 9032c38 into zenstruck:2.x Feb 1, 2025
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Maker] Don't try to set read only properties
2 participants